Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Probe for rm3100 #10581

Closed
wants to merge 3 commits into from
Closed

Probe for rm3100 #10581

wants to merge 3 commits into from

Conversation

khancyr
Copy link
Contributor

@khancyr khancyr commented Feb 20, 2019

No description provided.

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TMRC and CMM don't look right

@@ -117,13 +117,15 @@ bool AP_Compass_RM3100::init()
ccy1 != CCP1_DEFAULT || ccy0 != CCP0_DEFAULT ||
ccz1 != CCP1_DEFAULT || ccz0 != CCP0_DEFAULT) {
// couldn't read one of the cycle count registers or didn't recognize the default cycle count values
goto fail;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't seem to do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it just remove the goto that is useless

libraries/AP_Compass/AP_Compass_RM3100.cpp Outdated Show resolved Hide resolved
dev->write_register(RM3100_TMRC_REG, TMRC, true); // cycle count z
dev->write_register(RM3100_CMM_REG, CMM, false); // CMM configuration
dev->write_register(RM3100_TMRC_REG, TMRC, true); // CMM data rate
dev->write_register(RM3100_CMM_REG, CMM, true); // CMM configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you flight tested the use of CMM? If so, can you please give a flight log?
It does look like we should enable it, but it needs testing, and we should use a valid TMRC value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use this code to test the sensors, and the reading looks fine and agree with other compass. Surely with wrong TMRC, it get back to default value . here are some logs with the RM3100 as compass2 with this code : https://drive.google.com/open?id=19pSJBCCyfYx-mhPedg6KyBzomxzbfQEy

@@ -117,13 +117,15 @@ bool AP_Compass_RM3100::init()
ccy1 != CCP1_DEFAULT || ccy0 != CCP0_DEFAULT ||
ccz1 != CCP1_DEFAULT || ccz0 != CCP0_DEFAULT) {
// couldn't read one of the cycle count registers or didn't recognize the default cycle count values
goto fail;
// hal.console->printf("RM3100: Unable to read the register for whoami test\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left-over?

@khancyr
Copy link
Contributor Author

khancyr commented Mar 1, 2019

@tridge here is two logs with this PR :

rm3100-01032019.zip

@auturgy
Copy link
Contributor

auturgy commented Mar 11, 2019

@auturgy
Copy link
Contributor

auturgy commented Apr 28, 2019 via email

@khancyr
Copy link
Contributor Author

khancyr commented May 22, 2019

@tridge ping on this ! This driver has successfully fly since some weeks already

@OXINARF OXINARF requested a review from tridge June 26, 2019 13:23
@OXINARF
Copy link
Member

OXINARF commented Aug 2, 2019

I have a version of this at https://github.com/OXINARF/ardupilot/commits/review/10581. I believe that's the correct code.

It seems there was some confusion here with the simple change to add the CMM register to the checked registers. The TMRC register should be set, otherwise the default update rate is too low.

@khancyr if you are OK with it, I'll push it here and then add this for the dev call.

@zyromskij
Copy link

Hi :) I've done a few flights with RM3100 (Compass 2). Works great 🙂 Mag biases are much smaller compared to onboard ones. Also, it seems to have slightly less noise. Compass was intentionally placed (together with CubeBlack) just above the noisy ESCs
(We also tried that some time ago with HERE GPS but it was toilet bowling like crazy)
Please let me know if more testing is needed. Here are the logs:

RM3100_logs_02_08_19.zip

@weaverlave
Copy link

Any news when the RM3100 will be compatible with ArduRover? I would like test this on a big agriculture rover.

@rbachtell
Copy link

rbachtell commented Sep 17, 2019 via email

@weaverlave
Copy link

PM me, I have a working latest master .apj I have been using on a scale rover.

On Sep 17, 2019, at 8:07 AM, weaverlave @.***> wrote: Any news when the RM3100 will be compatible with ArduRover? I would like test this on a big agriculture rover. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10581?email_source=notifications&email_token=AC3NAIHBG73KANTSJPBEOTDQKDXEBA5CNFSM4GYVHLHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD643GRI#issuecomment-532263749>, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3NAIEW7SKCAVK233YLZQLQKDXEBANCNFSM4GYVHLHA.

Can´t send a PM to your acount. Maybe you can email me: jensweber95@web.de Thanks alot!

@rbachtell
Copy link

@OXINARF @khancyr Any chance this could be merged soon? We have a new ArduPilot-centric manufacturer with 3100 I2c and UAVCAN units in prototype and integrated GPS/3100 on the drawing board. It would be very useful to have this in master when they go to beta testing. They have the probe compiled as do I with hours of testing and no issues. Let me know if I can be of any help, I would be more than happy to do whatever I can. Thanks, RB

@zyromskij
Copy link

Here is another package of flash logs from a successful flights - CubePilot with an RM3100 module running on the I2C bus in a quadcopter.
(magnetometer with id: 1122305) - apj. kindly provided by @rbachtell

Compass was deliberately placed 2-3cm above noise ESC - we tried that with ICM 9dof sensor and immediately after takeoff, we had to switch to manual mode since the interference was too big ;)

https://drive.google.com/file/d/1SnTKaqwUuK8Oh6Itl7fhxsQKs4tTJdme/view?usp=sharing

ADD_BACKEND(DRIVER_RM3100, AP_Compass_RM3100::probe(GET_I2C_DEVICE(i, HAL_COMPASS_RM3100_I2C_ADDR),
true, ROTATION_NONE));
}
FOREACH_I2C_INTERNAL(i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we probing interbal bus? for internal it should be an explicit line in hwdef.dat

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse my lack of understanding but how does this differ from all the other internal i2c probes in this file?

@rmackay9
Copy link
Contributor

This came up in the Copter-4.0 testing thread here: https://discuss.ardupilot.org/t/copter-4-0-0-rc3-available-for-beta-testing/50250/18 so I'll add it to the to-do list for Copter-4.0 (but not as a blocker)

@tridge
Copy link
Contributor

tridge commented Dec 20, 2019

@OXINARF I have merged your version to master
@khancyr sorry this took so long!

@tridge tridge closed this Dec 20, 2019
@khancyr
Copy link
Contributor Author

khancyr commented Dec 21, 2019

No worries ! Thanks for handling that !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants